-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add water_network instance #59
Conversation
af2e3e2
to
f784ca0
Compare
Hi @tristantc . The linter check failed.
|
Thanks for the feedback, @ZedongPeng. |
@@ -0,0 +1,5 @@ | |||
cont,T |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CSV
file looks small. Maybe instead of creating the CSV
file, we should define a dictionary in the Python script. This also applies to TU.csv
and feed.csv
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would like to scale this instance to more cases, so I rather leave all in CSVs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good addition. Now, does it run with the benchmark code?
@@ -3,3 +3,4 @@ | |||
hda = "hda" | |||
HDA = "HDA" | |||
equil = "equil" | |||
ue = "ue" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is ue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ue
stands for unit_exists
gdplib/water_network/README.md
Outdated
|
||
### Size | ||
|
||
| Component | pwl | quadratic | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the original one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chose not to upload the MINLP model because it's not a GDP model, but a GAMS model transformed into pyomo or are you asking about the model size table?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I incorporated the original model without any approximation.
@@ -0,0 +1,5 @@ | |||
cont,T |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would like to scale this instance to more cases, so I rather leave all in CSVs
== ue.flow[mt, unit] | ||
) | ||
|
||
elif approximation == "piecewise": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quadratic2
function fits original function better than quadratic
, but not zero at zero flowrate.
Best known objective: $348,340
quadratic
$346,672
quadratic2
$349,555
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either in the documentation or in the README explain what quadratic and quadratic2 are. I would recommend to rename the models to quadratic_zero_origin and quadratic_nonzero_origin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to hold onto both versions?
BigM and Hull MINLP reformulations work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few open comments to be addressed and then we can merge this
ue.streams, | ||
doc="TU streams flowrate", | ||
domain=pyo.NonNegativeReals, | ||
# bounds=lambda _,i,k:(TU.loc[unit,'L'],100) # Upper bound for the flow rate from Ruiz and Grossmann (2009) is 100 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not adding the same bound as the original instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The overall feed flow rate is less than 100.
It provides increased flexibility for multiple cases.
This pull request incorporates the
water_network
instance.Please review the changes and provide feedback. Thank you!